Conversation
| } | ||
|
|
||
| if (resList == null | ||
| if (Boolean.getBoolean(CHECK_EMTPY_RESPONSE) && resList == null |
There was a problem hiding this comment.
it goes through a Properties access so it is synchronized, maybe read it once per client instance to avoid an useless locking?
jimma
force-pushed
the
master
branch
2 times, most recently
from
d86f72a to
bb1f9d4
Compare
|
@rmannibucau Thanks for review. You are right. I improved the code a bit, please check if it's good now. |
|
Was more thinking about doing it in the constructor but works for me |
|
The problem with this patch is that the default behaviour is still to throw a NPE in CXF - i.e. remove the change to the test in systests/uncategorized and you get a NPE. Perhaps instead we need to revisit the original fix. |
jimma
changed the title
|
@coheigea I wrongly thought this was an specific issue and it isn't. Looked at again and found this is actually caused by the null value return for primitive type in InvocationHandler(ClientProxy) :
Please review again. Thanks. |
jimma
force-pushed
the
master
branch
2 times, most recently
from
10baa23 to
01389cf
Compare
jimma
changed the title
| } else { | ||
| result = invokeSync(method, oi, params); | ||
| } | ||
| if (result == null && !method.getReturnType().equals(Void.TYPE) |
There was a problem hiding this comment.
nitpick: you can use == to check class equality
There was a problem hiding this comment.
Thanks for review. @andrei-ivanov . I agree with you, == is more direct for class equality.
| } | ||
|
|
||
| Object o = invokeSync(method, oi, params); | ||
| if (o == null && !method.getReturnType().equals(Void.TYPE) && method.getReturnType().isPrimitive()) { |
There was a problem hiding this comment.
nitpick: you can use == to check class equality
| throw new IllegalStateException("Response message did not contain proper response data. Expected: " | ||
| + boi.getOutput().getMessageParts().get(0).getConcreteName()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is all this code removed? If the WSDL states that there should be a response and there isn't, an exception should be thrown. The above is correct. By pushing some of this down into the ClientProxy's, anything that doesn't use the client proxies (like Camel endpoints) wouldn't have this check in place.
There was a problem hiding this comment.
Why is all this code removed? If the WSDL states that there should be a response and there isn't, an exception should be thrown. The above is correct.
When I ran reproducer EmptySoapBodyTest and found the NPE is thrown from Proxy class and root cause for this NPE is a null return value for a primitive type in InvocationHandler(ClientProxy), that's why I remove this and add this check in ClientProxy. I only looked at EmptySoapBodayTest , any other reproducer I need to have a look for CXF-7653 ?
Another reason why I remove these lines is this check doesn't work for this case: for example we have a simple wsdl which generates the following operation :
public HelloResponse doSomething(HelloRequest request);
To work with Dispatch/Provider , a provider class created to simply return an empty StreamSource like:
@WebServiceProvider(
serviceName="HelloService",
portName="HelloPort",
targetNamespace="http://cxf.apache.org/provider",
wsdlLocation="WEB-INF/hello.wsdl"
)
@BindingType(value=javax.xml.ws.soap.SOAPBinding.SOAP11HTTP_BINDING)
@ServiceMode (value=javax.xml.ws.Service.Mode.PAYLOAD)
public class HelloProvider implements Provider<Source> {
public Source invoke(Source req) {
return new StreamSource();
}
}
The client dispatch expects receiving a null value. Add this check in ClientImpl throws IllegalStateException instead of return a null result.
By pushing some of this down into the ClientProxy's, anything that doesn't use the client proxies (like Camel endpoints) wouldn't have this check in place.
For the caml endpoints without using the client proxy, it gets a null value result. Is the null value a problem for ClientCallback and something else ?
5 participants
… from provider endpoint